Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updating the API to a Swift3 style API #39

Merged
merged 1 commit into from
Oct 3, 2016
Merged

Updating the API to a Swift3 style API #39

merged 1 commit into from
Oct 3, 2016

Conversation

plivesey
Copy link
Owner

No description provided.

@plivesey
Copy link
Owner Author

API changes:

// protocol Model and protocol SimpleModel

func isEqualToModel(_ model: Model) -> Bool {}
func isEqual(to model: Model) -> Bool {}

func mergeModel(_ model: Model) -> Model
func merge(_ model: Model) -> Model

// DataProvider, CollectionDataProvider, BatchDataProviderListener

open var paused: Bool
open var isPaused: Bool

// DataProvider, CollectionDataProvider

open func fetchDataFromCache(cacheKey cacheKey: String?, ...)
open func fetchDataFromCache(withCacheKey cacheKey: String?, ...)

// CollectionDataProvider

open func removeAtIndex(_ index: Int, shouldCache: Bool = true, context: Any? = nil)
open func remove(at index: Int, shouldCache: Bool = true, context: Any? = nil)

Please also pay attention to all the APIs that I did not change.

@plivesey plivesey mentioned this pull request Sep 22, 2016
Closed
@codecov-io
Copy link

codecov-io commented Sep 22, 2016 •
edited
Loading

Current coverage is 90.01% (diff: 94.81%)

Merging #39 into master will decrease coverage by 0.06%

@@             master        #39   diff @@
==========================================
  Files            43         43          
  Lines          4538       4538          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           4088       4085     -3   
- Misses          450        453     +3   
  Partials          0          0

Powered by Codecov. Last update 95376f2...2704840

nicksnyder
nicksnyder approved these changes Oct 3, 2016
View reviewed changes
RocketData/CollectionDataProvider.swift
@@ -156,7 +156,7 @@ open class CollectionDataProvider<T: SimpleModel>: ConsistencyManagerListener, B
- parameter completion: Called on the main thread. This is called with the result from the cache.
At this point, the data provider will already have new data, so there's no need to call setData.
*/
open func fetchDataFromCache(cacheKey: String?, context: Any? = nil, completion: @escaping ([T]?, NSError?)->()) {
open func fetchDataFromCache(withCacheKey cacheKey: String?, context: Any? = nil, completion: @escaping ([T]?, NSError?)->()) {
Copy link
Contributor

@nicksnyder nicksnyder Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to consider:
fetchCachedData(withKey cacheKey: String?, ...)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm...originally liked, but maybe not...I think I prefer cache key because it's consistent naming across different places?

RocketData/CollectionDataProvider.swift
@@ -295,7 +295,7 @@ open class CollectionDataProvider<T: SimpleModel>: ConsistencyManagerListener, B
- parameter shouldCache: If false, we will not persist this to the cache.
- parameter context: This context will be passed onto the cache delegate. Default nil.
*/
open func removeAtIndex(_ index: Int, shouldCache: Bool = true, context: Any? = nil) {
open func remove(at index: Int, shouldCache: Bool = true, context: Any? = nil) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this should have a noun

removeItem(at index: Int, ...)
removeData(at index: Int, ...)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so based off Apple's APIs. Apple specifically suggests against this in API guidelines https://swift.org/documentation/api-design-guidelines/ (Omit needless words)

from ^
bad public mutating func removeElement(_ member: Element) -> Element?
good public mutating func remove(_ member: Element) -> Element?

@plivesey plivesey merged commit 65597af into master Oct 3, 2016
@plivesey plivesey deleted the swift3api branch October 3, 2016 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicksnyder nicksnyder nicksnyder approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@plivesey @codecov-io @nicksnyder